Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: simpler / more flexible api #181

Merged
merged 136 commits into from
Nov 14, 2024
Merged

chore: simpler / more flexible api #181

merged 136 commits into from
Nov 14, 2024

Conversation

manuel3108
Copy link
Member

@manuel3108 manuel3108 commented Oct 20, 2024

Relates #85

This is a POC to start visualizing stuff and initiate discussions. Besides of some new types & adapting the paraglide adder, I haven't done any implementation. So expect plenty of type errors and failing pipelines.

Up until now I was keeping my mouth shut about #85 as I had many doubts if this was the right way going forward. Also I had a hard time imagining how this would look in a real adder. Now that this is done with this POC it was eye opening how many flexibilities this will bring us & the community. Besides copying some code, there are nearly zero changes that affect the functionality of the adders. Since most of the functionality already exists it should be straight forward to implement this, once we come up with the "final" api. Most of the work probably needs to be done inside the adders, as there is lots of stuff that needs to be moved.

Edit: The diff looks way worse than it actually is. Consider opening the whole file or viewing in VScode

Copy link

changeset-bot bot commented Oct 20, 2024

⚠️ No Changeset found

Latest commit: a3b6954

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

pkg-pr-new bot commented Oct 20, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/sveltejs/cli/sv@181
pnpm add https://pkg.pr.new/sveltejs/cli/svelte-migrate@181

commit: a3b6954

@@ -10,28 +10,12 @@ import {
object,
variables,
exports,
kit
kit as kitJs
Copy link
Member Author

@manuel3108 manuel3108 Oct 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is bad, but works for the moment. This is required since our workspace has a kit property and this import is also named kit. What would be the best solution for something like this?

@AdrianGonz97
Copy link
Member

this really does look and feel much nicer!!

there are still things i would love to improve after this, such as using an ast manipulating api like this:

const { script, css, generateCode } = parseSvelte(content);
script.importNamed('../adder-template-demo.txt?raw', { demo: 'Demo' });
css.addClass('foobar', { 'background-color': 'blue' });
return generateCode({ script: script.generateCode(), css: css.generateCode() });

after that, i think we'll be super close to having a library that's ready for community integrations

@manuel3108
Copy link
Member Author

Regarding that bit of code that you removed back to the top: I moved it on purpose because it's only used in that one place and nowhere else, so i thought it was unnecessary polluting global stuff. But whatever works for you, i don't have a strong opinion on that.

const { script, css, generateCode } = parseSvelte(content);
script.importNamed('../adder-template-demo.txt?raw', { demo: 'Demo' });
css.addClass('foobar', { 'background-color': 'blue' });
return generateCode({ script: script.generateCode(), css: css.generateCode() });

I agree, there is lot's of improvement potential in the ast apis as well. Overall i like where this would be going. But as you said, one step at a time.

@AdrianGonz97
Copy link
Member

Regarding that bit of code that you removed back to the top: I moved it on purpose because it's only used in that one place and nowhere else, so i thought it was unnecessary polluting global stuff. But whatever works for you, i don't have a strong opinion on that.

yea, i much prefer keeping actual constants in the top scope, especially in this case so it's not polluting the sections with logic

@AdrianGonz97
Copy link
Member

AdrianGonz97 commented Oct 20, 2024

so one issue with this new api is that we'll no longer be able to determine if an integration has been previously installed as the packages are no longer static values that we can easily examine.

see:

// check if the dependent adder has already been installed
let installed = false;
installed = dependent.packages.every(
// we'll skip the conditions since we don't have any options to supply it
(p) => p.condition !== undefined || !!workspace.dependencyVersion(p.name)
);
if (installed) continue;
// prompt to install the dependent
const install = await p.confirm({
message: `The ${pc.bold(pc.cyan(adder.id))} integration requires ${pc.bold(pc.cyan(depId))} to also be setup. ${pc.green('Include it?')}`
});
if (install !== true) {
p.cancel('Operation cancelled.');
process.exit(1);
}
selectedAdders.push({ type: 'official', adder: dependent });
}

we're accessing the packages field and checking the package.json to see if they're installed.

the lucia integration relies on this to determine whether we should prompt to also install drizzle. this prevents the drizzle adder from being needlessly executed again if drizzle is already present.

not exactly sure how this can be worked around yet, but this is something we definitely needs to solve

@manuel3108
Copy link
Member Author

so one issue with this new api is that we'll no longer be able to determine if an integration has been previously installed as the packages are no longer static values that we can easily examine.

I wasn't aware anymore that we had this in place.

I was initially going to suggest something like this

run: ({ sv, cwd, options, typescript, kit, dependencyVersion }) => {
    if(!!dependencyVersion('drizzle-orm'))
        sv.dependsOn('drizzle)

    // do other file updates
}

That would increase flexibility as well. But this won't work because we will only be able to determine the integrations it depends on while executing the adder. But in general I do think a functional approach like this would be best, maybe by converting our existing dependsOn field into a function and basically doing the same thing as above.

@manuel3108
Copy link
Member Author

I just pushed a rough implementation of the new api (without "dependsOn" detection). This obviously requires more cleanup, but i wanted to make sure we don't miss anything.

Running the paraglide adder in it's current state seems to create the same results as our current implementation. Only had a rough look though, should probably diff the changes later for to make sure this is the case.

@manuel3108
Copy link
Member Author

FYI we have talked about nuking the routify adder in the past multiple times. Instead of rewriting it, that's what i have done right now.

@manuel3108
Copy link
Member Author

Update PR description to not close the referenced issue, so that we can focus on the other stuff later on.

packages/core/adder/config.ts Outdated Show resolved Hide resolved
packages/cli/commands/add/preconditions.ts Outdated Show resolved Hide resolved
Copy link
Member

@AdrianGonz97 AdrianGonz97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pretty satisfied with how this came out! I think it's finally ready to be merged

note: I decided that renaming everything to addon should probably happen in a follow up PR so we don't further pollute this one

@manuel3108 manuel3108 merged commit 9da497f into main Nov 14, 2024
8 checks passed
@manuel3108 manuel3108 deleted the chore/simpler-api-v2 branch November 14, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Successfully installed dependencies" despite failing to install eslint on an outdated node
3 participants